Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE-71] Hash keys and serialize/compress payload responses #72

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jlurena
Copy link
Collaborator

@jlurena jlurena commented Oct 15, 2024

No description provided.

Copy link
Owner

@mhgbrown mhgbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I left some questions for you.

cached_resource.gemspec Outdated Show resolved Hide resolved
@@ -104,7 +104,7 @@ def is_any_collection?(*arguments)
# Read a entry from the cache for the given key.
def cache_read(key)
object = cached_resource.cache.read(key).try do |json_cache|
json = ActiveSupport::JSON.decode(json_cache)
json = ActiveSupport::JSON.decode(MessagePack.unpack(json_cache))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about making packing/unpacking another configurable option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah thats what I was asking whether we should maintain it configurable or not. I can't think why anyone would not want this though.

If we make it configurable, we would have to require this dynamically based on the configuration option, which IMO is not an issue - just couldn't decide on whether this should be the norm or not.

I'll let you decide, configurable or norm? @mhgbrown

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go configurable! Thanks for laying it out for me.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jlurena, anything left to be done here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah the configuration part been a bit busy sorry

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, thanks for the udpate. I see you made another commit. I'll check it out...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Did as you asked.

Hey do you have discord or something? I'm hoping you can help me setup a pre version of cached_resource that makes use of this version of ActiveResource:

rails/activeresource#409

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey yeah, my handle is mhgbrown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants